Skip to content

fix(syncer): reject non-advancing message page cursors - #192

Merged
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/f003-message-cursor-stuck
Aug 31, 2026
Merged

fix(syncer): reject non-advancing message page cursors#192
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/f003-message-cursor-stuck

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 29, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

A full message page with a repeated or missing cursor can leave bootstrap, forward sync, or backfill looping. The submitted backfill guard also saved an invalid checkpoint before rejecting the page and was bypassed by bounded passes. The repaired implementation validates each full backfill page before saving its resume cursor, including one-page syncs, so the last usable checkpoint survives an error.

This incorporates the credited repair from triage/192-message-cursors after #181. Both catalog and message pagination regressions remain, with both sync documentation and Unreleased changelog entries. The original contributor commits and co-author credit are preserved. Two existing test operation deadlines now start after SQLite fixture setup, preserving their timeout assertions under load.

Validation uses the built CLI, production Discord HTTP client, and disposable SQLite stores against synthetic loopback REST responses. Repeated bootstrap, forward, and backfill cursors stop after two message requests; missing cursors stop after one; the previous backfill checkpoint remains intact; bounded malformed pages are rejected; and a healthy three-message page succeeds. Both this smoke proof and #181's catalog smoke proof are rerun on the final head. This is controlled HTTP integration proof, not a claim of a live Discord incident. Final-head CI and proof results are recorded in the maintainer landing comment.

@SebTardif
fix(syncer): reject non-advancing message page cursors
1b8b3fa 
A full 100-message page whose last or newest ID is empty or repeats
made bootstrap, forward, and unlimited backfill reprint forever.
Fail closed on a stuck before/after cursor, matching GuildMembers.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 31, 2026, 12:00 AM ET / 04:00 UTC.

ClawSweeper review

What this changes

The PR makes Discord message-history bootstrap, forward sync, and unlimited backfill return an error when a full page cannot advance its pagination cursor.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 7 items remain

Keep this PR open: its cursor guards address a real infinite-pagination path, but unlimited backfill saves an invalid cursor before rejecting it, and the supplied proof exercises a mock client rather than the production Discord request boundary.

Priority: P1
Reviewed head: afb8466d59578d0aad939e42602aa4bed64ced24

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The intended guard is useful, but a persistent checkpoint defect and mock-only proof make this PR not merge-ready.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The supplied macOS transcript runs the new Syncer test after the patch, but that test replaces ChannelMessages with repeatingMessagePageClient rather than exercising the production internal/discord.Client session request boundary. Provide a redacted real-client request/response trace for a malformed full page and its observed cursor error; redact tokens, endpoints, and other private data, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The supplied macOS transcript runs the new Syncer test after the patch, but that test replaces ChannelMessages with repeatingMessagePageClient rather than exercising the production internal/discord.Client session request boundary. Provide a redacted real-client request/response trace for a malformed full page and its observed cursor error; redact tokens, endpoints, and other private data, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed 5 items Introduced backfill ordering: The introduced backfill path writes nextBefore to persistent sync state at line 525, then only rejects an empty or non-advancing cursor at lines 537-542; an empty full page therefore overwrites the stored resume cursor before the function returns its new error.
Persistent-state consequence: SetSyncState accepts an empty cursor and upserts it; the next full sync reads that backfill cursor and uses it to choose the resume request.
Proof is mock-only: The added test's repeatingMessagePageClient replaces the Syncer ChannelMessages dependency, so the supplied compiled-test transcript does not exercise internal/discord.Client's real session request path.
Findings 1 actionable finding [P1] Validate the backfill cursor before checkpointing it
Security None None.

How this fits together

The syncer fetches Discord channel-message pages and persists messages plus resume cursors in the local crawl store. The cursor selects each subsequent request during bootstrap, incremental sync, and full-history backfill.

flowchart LR
  A[Discord message page] --> B[Channel syncer]
  B --> C[Persist messages]
  C --> D[Validate next cursor]
  D --> E[Next page request]
  D --> F[Cursor error]
  C --> G[Local sync state]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The supplied macOS transcript runs the new Syncer test after the patch, but that test replaces ChannelMessages with repeatingMessagePageClient rather than exercising the production internal/discord.Client session request boundary. Provide a redacted real-client request/response trace for a malformed full page and its observed cursor error; redact tokens, endpoints, and other private data, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
  • Validate the backfill cursor before checkpointing it (P1) - A full backfill page with an empty tail ID is written to channelBackfillScope before the new validation at lines 537-542 returns an error. SetSyncState accepts that empty value, replacing the last resumable checkpoint; validate first, then write only a known-good cursor, and assert the prior checkpoint remains intact on failure.
  • Resolve merge risk (P1) - A malformed full backfill page can replace the durable resume cursor with an empty value before failing, so a later full sync may restart or recrawl instead of resuming from the last known-good checkpoint.
  • Resolve merge risk (P1) - The supplied test output proves the mocked syncer path only; it leaves real Discord-client request/response behavior unproven before merge.
  • Complete next step (P2) - A narrow code-and-test repair can remove the checkpoint-ordering defect, while the contributor must separately provide real behavior proof before merge.
  • Improve patch quality - Validate the backfill cursor before its state write and add regression assertions that invalid full pages do not replace the previous checkpoint.
  • Improve patch quality - Provide redacted production-boundary request/response evidence using the real Discord client path.

Findings

  • [P1] Validate the backfill cursor before checkpointing it — internal/syncer/message_sync.go:525
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 2 files; production +25/-4, tests +163 The change modifies all three message pagers and adds six cursor-error scenarios, so durable backfill-state coverage is material.

Merge-risk options

Maintainer options:

  1. Complete cursor validation before checkpointing (recommended)
    Move the unlimited-backfill cursor checks ahead of the sync-state write, add coverage that the prior checkpoint survives invalid pages, and attach redacted production-boundary proof before merge.

Technical review

Best possible solution:

Validate the unlimited-backfill cursor before writing its checkpoint, preserve the last known-good cursor on failure, cover that durable-state invariant, and provide a redacted real-client request/response trace showing the cursor error.

Do we have a high-confidence way to reproduce the issue?

Yes, at the source level: the added client returns a repeated or empty full page and demonstrates the affected pager loop condition. The supplied evidence does not yet reproduce that response through the real Discord transport boundary.

Is this the best way to solve the issue?

No; the cursor checks are the right repair direction, but unlimited backfill must reject the cursor before persisting it and the fix still needs production-boundary evidence.

Full review comments:

  • [P1] Validate the backfill cursor before checkpointing it — internal/syncer/message_sync.go:525
    A full backfill page with an empty tail ID is written to channelBackfillScope before the new validation at lines 537-542 returns an error. SetSyncState accepts that empty value, replacing the last resumable checkpoint; validate first, then write only a known-good cursor, and assert the prior checkpoint remains intact on failure.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 4c29bb75ef7f.

Labels

Label justifications:

  • P1: A full-page cursor failure can currently make an active channel-history sync hang, and the proposed repair still has a checkpoint-safety defect.
  • merge-risk: 🚨 session-state: The introduced backfill path writes an invalid resume cursor into persistent sync state before returning an error.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The supplied macOS transcript runs the new Syncer test after the patch, but that test replaces ChannelMessages with repeatingMessagePageClient rather than exercising the production internal/discord.Client session request boundary. Provide a redacted real-client request/response trace for a malformed full page and its observed cursor error; redact tokens, endpoints, and other private data, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] go test ./internal/syncer -count=1 -run TestMessagePagesErrorWhenCursorDoesNotAdvance.
  • [P1] go test ./internal/syncer -count=1.
  • [P1] Capture a redacted real Discord-client request/response trace showing an invalid full page returns the cursor error and leaves the prior backfill checkpoint unchanged.

What I checked:

  • Introduced backfill ordering: The introduced backfill path writes nextBefore to persistent sync state at line 525, then only rejects an empty or non-advancing cursor at lines 537-542; an empty full page therefore overwrites the stored resume cursor before the function returns its new error. (internal/syncer/message_sync.go:525, afb8466d5957)
  • Persistent-state consequence: SetSyncState accepts an empty cursor and upserts it; the next full sync reads that backfill cursor and uses it to choose the resume request. (internal/store/write.go:577, afb8466d5957)
  • Proof is mock-only: The added test's repeatingMessagePageClient replaces the Syncer ChannelMessages dependency, so the supplied compiled-test transcript does not exercise internal/discord.Client's real session request path. (internal/syncer/message_sync_cursor_test.go:23, afb8466d5957)
  • Prior review continuity: The latest completed ClawSweeper review already identified the same P1 checkpoint-ordering defect; the current head is unchanged, so the concern remains unresolved rather than newly introduced by review drift. (internal/syncer/message_sync.go:525, afb8466d5957)
  • Area history: Current syncer history includes recent message-sync work by Ayaan Zaidi and checkpoint work by Hannes Rudolph, making them reasonable routing candidates for this repair. (internal/syncer/message_sync.go:323, 5a0dc6f611f8)

Likely related people:

  • Ayaan Zaidi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Hannes Rudolph: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (5 earlier review cycles)
  • reviewed 2026-08-29T18:16:15.590Z sha 1b8b3fa :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-30T01:22:15.738Z sha afb8466 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-30T04:00:38.499Z sha afb8466 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-30T09:53:15.122Z sha afb8466 :: needs real behavior proof before merge. :: [P1] Validate the backfill cursor before checkpointing it
  • reviewed 2026-08-30T17:04:06.748Z sha afb8466 :: needs real behavior proof before merge. :: [P1] Validate the backfill cursor before persisting it

@SebTardif
fix(syncer): use strconv.Itoa in cursor test IDs
afb8466 
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper clawsweeper Bot added merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 30, 2026
@steipete

steipete commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Maintainer triage: LAND recommended using the credited, repaired implementation on triage/192-message-cursors, commits 011d239 and ef0475b. No merge performed; the contributor fork remains unchanged.

The loop is reproducible. The submitted backfill guard also saved an empty checkpoint before rejecting the page, and its bounded path bypassed the guard entirely. The repair validates a full page before saving its backfill cursor, including one-page syncs. It retains the existing short-page and --since behavior, adds durable-state regression coverage, and includes documentation/changelog credit to @SebTardif.

Built-CLI proof used real cmd/discrawl, the production Discord HTTP client, and SQLite against a synthetic loopback REST fixture. Only the Discord base URL was changed at link time. No live Discord incident or real account data is claimed.

GOWORK=off go build -ldflags '-X github.com/bwmarrin/discordgo.EndpointDiscord=http://127.0.0.1:38082/' -o /tmp/discrawl ./cmd/discrawl
python3 message-smoke.py /tmp/discrawl

Before: bootstrap/forward/backfill repeated or missing cursors reached the 6-request fixture cap; bounded malformed page was accepted.
After:
  bootstrap repeated: 2 requests, cursor error
  bootstrap missing: 1 request, missing-ID error
  forward repeated: after=2500 -> after=3000, 2 requests, cursor error
  backfill repeated: before=4000 -> before=2901, 2 requests, cursor error
  backfill missing: 1 request, cursor 4000 preserved
  bounded missing: 1 request, error, no invalid checkpoint
  healthy short page: exit 0, 3 messages stored

An independent source-blind validator reproduced the before defects and passed all seven repaired cases. Focused cursor/checkpoint tests and go vet ./internal/syncer passed. Codex autoreview was clean for the production repair and the separate test-only cleanup.

A broader syncer run exposed two pre-existing five-second timers expiring during SQLite fixture setup. Moving those operation deadlines after fixture setup preserves the actual timeout assertions; both previously failing tests now pass. Full-suite validation encountered CLI/share package timeouts under local load; lower-concurrency follow-up results will be included in the final handoff. Existing PR CI is green but does not cover this maintainer branch.

@steipete @SebTardif
fix(syncer): preserve checkpoints when message cursors stall
906742d 
Merge landed catalog cursor guards and apply the validated message pagination repair, including bounded backfill checkpoint validation and durable-state regression tests. Retain both documentation and changelog entries and move the two existing operation deadlines after fixture setup.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete

steipete commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Maintainer landing verification for 906742dc5d58bdc9384ada1bb43dc8e305064b5a, incorporating landed #181 (525b637):

The contributor branch now contains the validated checkpoint repair as a new commit, preserving contributor history and co-author credit. Both catalog and message regression sets, sync documentation, and changelog thanks remain. Codex autoreview returned no actionable findings. GOWORK=off go test -p 1 ./internal/syncer -count=1 passed (105.838s); the Discord suite also passed for the unchanged catalog repair.

Both CLI binaries were rebuilt from this exact final head. Only the Discord base URL was changed at link time. The real CLI, production HTTP client, and disposable SQLite stores exercised synthetic loopback REST fixtures:

Catalog smoke:
  repeated guild: exit 1, 2 guild requests, cursor error
  missing guild ID: exit 1, 1 guild request, missing-ID error
  repeated archive: exit 0, 2 archive requests, cursor warning
  healthy guild: exit 0, guilds=1

Message smoke:
  bootstrap repeated: exit 1, 2 requests, cursor error
  bootstrap missing: exit 1, 1 request, missing-ID error
  forward repeated: after=2500 -> after=3000, 2 requests, cursor error
  backfill repeated: before=4000 -> before=2901, 2 requests, cursor error
  backfill missing: exit 1, 1 request, checkpoint 4000 preserved
  bounded missing: exit 1, 1 request, no invalid checkpoint
  healthy short page: exit 0, 1 request, 3 messages stored

All eleven scenarios passed. Archive catalog failures retain the existing best-effort warning policy. This is controlled HTTP integration proof, not a claim that live Discord returned malformed pages.

@steipete
steipete merged commit 1359f06 into openclaw:main Aug 31, 2026
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete